Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix file watcher regression #28

Merged
merged 2 commits into from
Dec 13, 2021
Merged

Conversation

ikappaki
Copy link
Contributor

Hi,

could you please consider fix for issue #27.

It is caused by the recent #23 commit which uses a regexp against the modified filename to determine which files are supported to be shown by the browser.

The original commit converted the modified file's Path into an io/file for convenience and then extracted the filename as a String, but the Path to io/file conversion fails on OS X throwing an exception (as shown at the end).

The fix is to retrieve the filename string from the Path directly.

Also, it might be a good idea to have clerk/file-event try and catch any exception thrown instead of silently failing?

error thrown: (is this a bug?)

:error #error {
 :cause No implementation of method: :as-file of protocol: #'clojure.java.io/Coercions found for class: sun.nio.fs.UnixPath
 :via
 [{:type java.lang.IllegalArgumentException
   :message No implementation of method: :as-file of protocol: #'clojure.java.io/Coercions found for class: sun.nio.fs.UnixPath
   :at [clojure.core$_cache_protocol_fn invokeStatic core_deftype.clj 583]}]
 :trace
 [[clojure.core$_cache_protocol_fn invokeStatic core_deftype.clj 583]
  [clojure.java.io$fn__11390$G__11372__11395 invoke io.clj 35]
  [clojure.java.io$file invokeStatic io.clj 424]
  [clojure.java.io$file invoke io.clj 418]
  [nextjournal.clerk$supported_file_QMARK_ invokeStatic NO_SOURCE_FILE 181]
  [nextjournal.clerk$supported_file_QMARK_ invoke NO_SOURCE_FILE 174]
  [nextjournal.clerk$file_event invokeStatic clerk.clj 211]
  [nextjournal.clerk$file_event invoke clerk.clj 209]
  [nextjournal.clerk$serve_BANG_$fn__18760 invoke clerk.clj 289]
  [nextjournal.beholder$fn$reify__6032 onEvent beholder.clj 13]
  [io.methvin.watcher.DirectoryWatcher onEvent DirectoryWatcher.java 352]
  [io.methvin.watcher.DirectoryWatcher watch DirectoryWatcher.java 297]
  [io.methvin.watcher.DirectoryWatcher lambda$watchAsync$0 DirectoryWatcher.java 212]
  [java.util.concurrent.CompletableFuture$AsyncSupply run CompletableFuture.java 1768]
  [java.util.concurrent.CompletableFuture$AsyncSupply exec CompletableFuture.java 1760]
  [java.util.concurrent.ForkJoinTask doExec ForkJoinTask.java 373]
  [java.util.concurrent.ForkJoinPool$WorkQueue topLevelExec ForkJoinPool.java 1182]
  [java.util.concurrent.ForkJoinPool scan ForkJoinPool.java 1655]
  [java.util.concurrent.ForkJoinPool runWorker ForkJoinPool.java 1622]
  [java.util.concurrent.ForkJoinWorkerThread run ForkJoinWorkerThread.java 165]]}

(Tested it to work on both OS X and MS Windows)

Thanks!

Attempting to convert Path to io/file first fails on OS X.
@ikappaki
Copy link
Contributor Author

It appears clojure.java.io/file does not support Paths as an argument to begin with, so original implementation was incorrect trying to coerce Path into a File. It should fail on all architectures, not just OS X.

https://clojure.atlassian.net/browse/CLJ-2333

io/file delegates to the io/Coercions protocol's as-file method for coercing its arguments into a File. It can coerce String, File, URL and URIs by default.

It so happens on my main dev machine I have clj-refactor installed, which injects the refactor-nrepl into the repl, which inlines clj-commans/fs (with MrAnderson) , which extends io/Coercions with Path, and thus how it was tested to work for me in the first place ..., e.g. in my REPL with clj-refactor installed , I get:

user> (:impls clojure.java.io/Coercions)
{nil
 {:as-file #function[clojure.java.io/fn--11402],
  :as-url #function[clojure.java.io/fn--11404]},
 java.lang.String
 {:as-file #function[clojure.java.io/fn--11406],
  :as-url #function[clojure.java.io/fn--11408]},
 java.io.File
 {:as-file #function[clojure.java.io/fn--11410],
  :as-url #function[clojure.java.io/fn--11412]},
 java.net.URL
 {:as-url #function[clojure.java.io/fn--11414],
  :as-file #function[clojure.java.io/fn--11416]},
 java.net.URI
 {:as-url #function[clojure.java.io/fn--11418],
  :as-file #function[clojure.java.io/fn--11420]},
 java.nio.file.Path
 {:as-file
  #function[refactor-nrepl.inlined-deps.fs.v1v6v307.me.raynes.fs/eval2296/fn--2297],
  :as-url
  #function[refactor-nrepl.inlined-deps.fs.v1v6v307.me.raynes.fs/eval2296/fn--2299]}}

I suppose having REPL middleware modifying core behaviour could be considered dangerous, though it happens indirectly byclj-commns/fs. I shall reach out to them to see what they think.

I originally thought clerk/file-event's path was a string and thus the original implementation with passing it to io/file and the unit tests written with strings. I suppose in retrospect I should have checked the path's type. and not relied on the REPL#s behaviour...

Thanks!

@mk
Copy link
Member

mk commented Dec 13, 2021

Thanks for looking into this again and digging so deep to figure out what the problem is. I also do have refactor-clj on the classpath and hence everything was working for me when I tested it.

Will look into setting up a test suite (with a clear separation of the test and dev class paths) soon.

@mk mk changed the title Fix for OS x issue with watcher failing to refresh modified files. Fix file watcher regression Dec 13, 2021
@mk mk merged commit 8ad8863 into nextjournal:main Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants